fix(proxy): survive a reload instead of cutting the response - #304
Conversation
8afb859 to
6cb7295
Compare
808cc80 to
41e2d96
Compare
8f7db16 to
cf1d7fe
Compare
|
Heads up: your branch has an accumulated 15 workflow runs in Practical impact on the queue: on the free-tier concurrency budget those runs are holding runner slots, and everything behind them is backing up. Two asks, both about future pushes rather than the accumulated ones (which need a maintainer with
Not blocking on your PR review — this is drafting-time hygiene, not a review finding. Ping when the hang is either reproduced locally or ruled out, and we'll figure out the next step from there. — Proxy Builder |
5406c3a to
a2363e1
Compare
…e wrong hop
Twelve findings from the review of this PR, plus one raised by cswap's pin
against the fix for the last of them. Each has a test that dies when its fix is
reverted.
THE DEPLOY ONES, which is why this is not a tidy-up:
otherHolderOn() compared process AGE only. Every incumbent outlives a process
that just started, so on every deploy the NEW code judged itself surplus, exited
0, and the OLD holder kept serving with nothing saying so. Now a holder is a
duplicate only when it is running the same code.
holderPidOn() answered "holder" on the mere presence of a run-service, which
made runningOurCode() unreachable: the holder always keeps a descriptor to the
listening socket, so the loop always returned before the fingerprint branch.
bindFailed() read no error code, so a bind that can NEVER work — an address not
on this host, a privileged port — took the "someone else has it" path, found no
incumbent to ask, and exited 0. A deploy that started nothing reported success.
Bind errors also carried libuv's errno through two hardcoded literals that named
EADDRINUSE and called everything else EACCES; util.getSystemErrorName is right
on both platforms and for every code.
The holder matched its child's release announcement against a RAW CHUNK while
the port line beside it was line-buffered. A chunk boundary inside "(handed
off)" reads a handover as a plain release, so the holder reclaims the port from
the successor already serving on it and spawns a second — the failure the
(handed off) marker exists to prevent, re-entered through the marker itself.
Ownership probes asked lsof about 127.0.0.1 while the bind honoured
CACHE_FIX_PROXY_BIND. Under any other address the probe matched nothing.
CACHE_FIX_PROXY_PORT=0 was rewritten to the legacy 9801 by `Number(env) || 9801`
("0" is a truthy string), while proxy/config.mjs read the same variable with
envInt and yielded 0.
THE SHUTDOWN ONES:
shutdown() had no re-entry guard although it is bound to SIGTERM, SIGINT and
SIGHUP, and a control-group stop delivers more than one. Each entry can put
another successor on fd 3. The window only exists while something is draining,
which a live session always is.
handle.close() always rejected on that path, because shutdown() closes the
server one line earlier and the second close reports ERR_SERVER_NOT_RUNNING.
Only the process.exit() inside .finally() beat the unhandled-rejection report.
THE HOP ONES:
/health.https_proxy published a configured candidate. resolveHop() falls
THROUGH the chain, so it named ":8118" while CONNECTs left via the second
fallback or via nothing at all. It now publishes the hop a resolve actually
used, and null when the chain was checked and found dead.
cswap's pin raised that the fix left one field carrying two meanings — a URL is
either measured or merely configured and a reader cannot tell. Split into
https_proxy_measured, and direct_last: a sticky ISO instant of the last direct
fall-through, under the name and for the reason the pin uses. A chain flaps back
within ~1s, so a point-in-time field cannot report the outage that happened.
hopAlive() and parseProxy() defaulted an https:// hop with no explicit port to
80, so a live TLS hop read as dead and the chain fell through past it.
CONNECT fell open to a direct dial with no way to refuse. Fail-open stays the
default on both ends of the chain — a hop restarting is back in ~1s and refusing
strands a session whose HTTPS_PROXY was baked at exec — but CACHE_FIX_REQUIRE_HOP=1
now exists for a deployment where the hop is a policy boundary rather than a cache.
Ref cnighswonger#304
Co-Authored-By: Claude <noreply@anthropic.com>
…open Self-review of the previous commit. The opt-in lived in forward-proxy.mjs and covered the two CONNECT paths only, while forwardRequest() — the relayed /v1/messages path, which is what this proxy exists for — kept dialling direct with the variable set. A door that closes the tunnel and leaves the main path open reads as closed and is not, which is worse than leaving it open honestly. THE OBVIOUS FIX IS WORSE, AND THAT IS WHY THIS COMMIT DOES NOT SHIP IT. Throwing from forwardRequest() is caught by handleMessages, but its catch opens with `if (abortController.signal.aborted) return`, and that signal is wired to clientReq's own "close" — which Node emits when the request BODY completes, not only when the client goes away. Measured with the guard in place: hop="" requireHop=true env="1" caught: no chain hop reachable | aborted=true | writableEnded=false POST /v1/messages -> TIMEOUT (10016ms) The client is still there and gets nothing. A leak that is honest beats a hang that reads as a refusal, so the guard stays off that path. The abort listener is a pre-existing defect, not one this introduced, and there is no evidence it has ever worked: instrumenting the same catch and running the existing "POST /v1/messages routes to upstream" case printed nothing at all — that test gets a real 401 and never enters the catch. Fixing it means changing streaming-abort semantics for every request, which is the highest-risk edit in this file and does not belong in a follow-up to a deploy that has already shipped. So the scope is recorded rather than hidden: requireHop moves to upstream.mjs beside resolveHop with the measurement in its comment, and the test asserts the relayed path is NOT refused, with a message telling whoever fixes the abort listener to come back and update both. No behaviour change from the previous commit. Ref cnighswonger#304 Co-Authored-By: Claude <noreply@anthropic.com>
handleMessages installs an abort so a client that gives up mid-SSE frees the upstream. It was keyed on clientReq's "close" — which Node emits when the request BODY has been consumed, i.e. on every request, immediately — so it aborted while the client was still sitting there, and the forwardRequest catch opens with `if (aborted) return`. Nothing was written back. Measured in reverse mode on the real /v1/messages path, against an upstream that refuses instantly, which is what a dead local hop does: before body at once -> HANG (6s client timeout) body delayed -> HANG after body at once -> 502 in 9ms body delayed -> 502 in 52ms Keyed on clientRes's close instead: it fires when the response finishes OR the connection is destroyed, so pairing it with writableEnded separates "we answered" from "the client hung up". Same change in handlePassthrough, which carried the identical line. This was found by measuring an exposure I had already dismissed as too risky to touch. The previous commit recorded it as a latent defect blocking a different fix; it is not latent, it is on the most ordinary upstream failure there is. WHAT THE TESTS DO AND DO NOT GUARD, because the difference matters: the 502 case dies when the listener is reverted to clientReq — mutation-checked the no-leak case does NOT die when the listener is deleted outright Two attempts at the second: client takes a frame then leaves (the pipe tears the upstream down by itself), and an upstream that accepts and never answers so no pipe exists (still freed). Both passed with the listener removed. So the listener may be doing nothing that socket teardown does not already do. It stays — "I could not demonstrate it matters" is not "it does not matter" — and the case is labelled as pinning the PROPERTY, not guarding the listener, so nobody reads it as coverage it is not. ALSO: the relayed probe added in 70ff998 dialled the real api.anthropic.com, because that test never set CACHE_FIX_PROXY_UPSTREAM and the default is the live host. That is the trap integrated.conf line 20 already warns about, and it took CI red on node 22 while bafabae with identical proxy code was green. It now runs against a local 418, on its own instance — pointing config.upstream at loopback for the whole case makes the CONNECT half read the tunnel target as the upstream and stop blind-tunnelling it, which failed the fail-open assertion for an unrelated reason. Ref cnighswonger#304 Co-Authored-By: Claude <noreply@anthropic.com>
The two branches answer the same question — is another process listening on the port we advertise — and they disagreed about what counts. /proc matched on the PORT alone (f[1] ends with :hexport, any local address); lsof pinned the 127.0.0.1 literal. The lsof branch is the only one a mac reaches, and two of three machines here are macs. So a proxy bound anywhere other than loopback read as "no successor" forever, and the outgoing proxy waited out its whole 30s ceiling on every handover instead of leaving as soon as its successor served. Found by sweeping for siblings of a fix already shipped: the launcher's two ownership probes were taught to honour CACHE_FIX_PROXY_BIND, and this third one in server.mjs was missed. That is twice in one day that a fix landed on the call sites in the diff and not on the ones a grep would have found, which is the class of mistake this sweep exists to catch. Matched to /proc rather than teaching /proc the address: a wildcard listener (0.0.0.0) serves loopback traffic but does NOT match an `-iTCP@127.0.0.1` query, so an address filter has a blind spot of its own — and it is the blind spot that errs toward "a successor exists", which would let a proxy leave an unowned port behind. The test spawns the wildcard listener in ANOTHER process. A self-owned one answers false either way, because the function excludes its own pid, so the first version of this case passed against the literal it was written to catch. Mutation-checked: restoring 127.0.0.1 fails it. NOT CHANGED, checked and deliberately left: forward-proxy.mjs connectUpstreamTLS defaults the upstream port to 443 regardless of scheme. It tls.connect()s unconditionally, so 443 is the right default for what that function does; defaulting by scheme would send an http upstream to port 80 over TLS, which is worse. The real oddity there is TLS to a plain-http upstream, and that is not this PR's to change. Ref cnighswonger#304 Co-Authored-By: Claude <noreply@anthropic.com>
… that did not
The previous commit shipped the abort fix with an honest note that the no-leak
half was unguarded: two attempts at a case for it both passed with the listener
DELETED outright, so it could not be claimed as coverage. It is guarded now, and
both earlier attempts were wrong for reasons worth keeping.
WHAT IT ACTUALLY DOES. forwardRequest wires the signal to upstreamReq.destroy().
When the upstream has not yet ANSWERED there is no pipe for socket teardown to
travel along, so the abort is the only thing that can free the connection:
with the listener dialled 1, live 1 at walk-away -> 0 after 2s
listener deleted dialled 1, live 1 at walk-away -> 1 after 2s
WHY THE FIRST TWO FAILED, both my own defects:
1. No premise. The case asserted only "0 connections at the end", which a
proxy that never dialled satisfies just as well. It carried an
`assert.ok(x || true, "")` placeholder I had left in — an assertion that
cannot fail. It now asserts that the proxy dialled AND that the connection
was live at the moment the client left.
2. Process-global contamination. With the premise added it STILL passed inside
proxy-server.test.mjs while the identical logic in a process of its own
separated cleanly — cached keep-alive agents, a forward-mode instance's
self-heal, another startProxy winding down. So it moves to its own file,
the same reason proxy-holder-handover.test.mjs is one case alone.
AND THE MUTATION EXPOSED A THIRD DEFECT IN THE TEST. With the listener deleted
it first died at the runner's 120s timeout reporting `pass 0 fail 0`: the leaked
connection kept h.close() draining, cleanup hung, and the assertion message that
had already fired was lost. Cleanup destroys the upstream sockets first now, so
the mutation fails in 2.5s with something readable. A case that discriminates
only by timing out is one nobody can act on.
No production change. Ref cnighswonger#304
Co-Authored-By: Claude <noreply@anthropic.com>
The review agent I had reported dead came back after 7.7h. Of its 15 findings
two were already closed by later commits; these are the live ones that are mine.
THE PORT-0 FIX CREATED THIS. Removing `Number(env) || 9801` let
CACHE_FIX_PROXY_PORT=0 through, and two sites below the bind still passed the
REQUESTED port where the BOUND one is required — while the gap and standby a
couple of hundred lines up already used `this._port`.
publishFingerprint(port) wrote cache-fix-proxy-0.sha256, so
runningOurCode(<bound>) from any other
launcher finds nothing and every port-0
install on the box collides on one file
CACHE_FIX_HELD_PORT: String(port) told the child "0", so its self-heal would
respawn on a DIFFERENT ephemeral port and
strand every session on the served one,
and successorServing("0") can never answer
Measured with CACHE_FIX_PROXY_PORT=0: bound 43557, record named …-0, child told
0. After: record named for the bound port, child told the bound port. Both
halves mutation-checked separately.
TWO DEFECTS IN MY OWN NEW TESTS, both the process-global class:
The first cut asserted on /tmp/cache-fix-proxy-0.sha256 — a GLOBAL path. It
passed alone and failed in the full suite, because something else on the box
had created it. An assertion on a shared path measures the machine's history,
not the code. The holder now gets a private TMPDIR.
`announces its release exactly once` was starving its own file. node runs a
describe's subtests concurrently, and that case adds a full run-service holder,
a proxy child, an in-flight connection, a 3s settle and a cleanup loop that
SIGHUPs every pid on its port. The agent measured it: 7 full runs, 3 failures,
all in that file and varying between cases, against 0 in 3 with the case
excised. It moves to its own file — the remedy proxy-holder-handover.test.mjs
already applies to itself, for the same reason, in its own header. Still
mutation-checked in its new home.
Full suite now 3 consecutive runs, 0 failures, 1854/1853.
ALSO, from cswap's pin: a tripwire on the CONNECT case, because an assertion
that fires on `[]` has already discarded the evidence that would narrow it.
Every endpoint records now, so a failure says which was touched — measured
`["UPSTREAM"]` when the tunnel is aimed there, `[]` when it reached none. The
comment says plainly that `[]` still does not name the third case (the proxy
MITM'ing the target itself), because narrowing is not naming.
STILL OPEN, recorded not fixed: CACHE_FIX_REQUIRE_HOP closes two of four
fall-open egress paths. bin/gap-relay.mjs direct() does not consult it at all,
and that is the tunnel that carries traffic precisely when the proxy is down.
The pin's own _blind_tunnel walks its chain per hop, treats a non-200 as
"refused BY this hop", and reaches direct only when none will carry — with the
refusal traced. Their advice, which this does not yet implement: closing on
no-hop trades an invisible fall-open for an invisible outage.
Ref cnighswonger#304
Co-Authored-By: Claude <noreply@anthropic.com>
It read the FIRST usable candidate at startup and fell straight to a direct dial
when that one would not carry, so a configured second hop was never tried.
proxy/upstream.mjs resolveHop() walks the whole list — one chain carrying two
definitions of itself, and the relay's copy is the one that runs precisely when
the proxy is down.
Measured, three recording endpoints and a relay per case:
hop1 dead, hop2 alive before: ORIGIN (dialled past a hop that would carry)
after: HOP2, 1 refusal traced
both hops dead after: ORIGIN, 2 refusals traced
NOT A HARD CLOSE when none will carry, and that is cswap's pin's call rather
than mine. I had proposed consulting CACHE_FIX_REQUIRE_HOP here; they measured
that closing on no-hop trades an invisible fall-open for an invisible outage,
and this is the tunnel that carries traffic when the proxy is down — the most
expensive place to take one. Their _blind_tunnel does the same walk, treats a
non-200 as "refused BY this hop", and reaches direct only when none will carry.
Direct stays the last resort; the refusals are traced so it is not a silent one.
The trace uses the string proxy/upstream.mjs already emits — `hop <addr>
unusable` — so one grep reads both ends of the chain.
Also retracted: I had argued this hole mattered because a direct route's leaf
carries no Authority Key Identifier. The pin corrected it — that applies to a
MITM leaf, not a blind tunnel carrying the client's own TLS to the origin. The
hole is real for a different reason: a bypass nobody can see in the log.
THE FIRST MEASUREMENT OF THIS SAID THE FIX DID NOTHING. Zero endpoints touched,
zero traces, both scenarios. The relay listens on `srv.listen({ fd: 3 })`
because the holder hands it an already-bound socket, and the fixture spawned it
without one — so it never listened, and a broken instrument read exactly like a
broken fix. The test now asserts `gap-relay carrying` as a premise before
measuring anything, so the next person gets "nothing was measured" instead of a
false negative.
Both halves mutation-checked separately: reverting the walk fails both cases,
and keeping the walk while dropping the trace fails both too.
Suite 1856/1855/0. Ref cnighswonger#304
Co-Authored-By: Claude <noreply@anthropic.com>
Three of the review agent's minor findings, verified rather than relayed. All three were mine. AN EMPTIED CHAIN LEFT A HOP BEHIND. Both getters read the env per call, so the list can go away under a running proxy, and resolveHop's `if (!chain.length) return ""` skipped _lastHop entirely. Measured: resolved :40559, chain emptied, resolveHop returned "" and lastHop() still said :40559 — so /health went on naming an address no request could take. That is the exact lie the field was fixed to stop telling, re-entering through an early return the fix did not touch. _directLast is deliberately NOT stamped there. "No chain was ever configured" is not a fall-through, because there was no chain to fall through; stamping it would fire on every reverse-mode proxy that never had one and empty the field of the meaning it exists for. Asserted, so the distinction survives a refactor. A COMMENT OF MINE WAS A LIE. Three tests set CACHE_FIX_CHAIN_GRACE_MS after importing upstream.mjs and called the retry loop "not what is under test". CHAIN_GRACE_MS is a module-level const captured at import, so the assignment does nothing — measured, the case runs 2,616 ms, one full 2,500 ms default window. Set BEFORE the module loads it works: 28 ms. So the knob is fine for an operator, who sets it before the proxy starts, and the production code is unchanged; the comment is what was wrong, and a comment telling the next reader the wait is gone is worse than the 2.5 s. THE SCHEME-PORT INVARIANT COVERED TWO COPIES OF THREE. bin/gap-relay.mjs carries its own portOf() because it imports node:net and nothing else — it runs when the proxy is DOWN, so depending on proxy/ modules would let a broken one take the relay with it. The duplication is deliberate; leaving it unchecked was not, and it was already correct there, which is why the other two read as a regression against it. Mutation-checked: breaking gap-relay's copy now fails the case. Suite 1856/1855/0. Ref cnighswonger#304 Co-Authored-By: Claude <noreply@anthropic.com>
…e wrong hop
Twelve findings from the review of this PR, plus one raised by cswap's pin
against the fix for the last of them. Each has a test that dies when its fix is
reverted.
THE DEPLOY ONES, which is why this is not a tidy-up:
otherHolderOn() compared process AGE only. Every incumbent outlives a process
that just started, so on every deploy the NEW code judged itself surplus, exited
0, and the OLD holder kept serving with nothing saying so. Now a holder is a
duplicate only when it is running the same code.
holderPidOn() answered "holder" on the mere presence of a run-service, which
made runningOurCode() unreachable: the holder always keeps a descriptor to the
listening socket, so the loop always returned before the fingerprint branch.
bindFailed() read no error code, so a bind that can NEVER work — an address not
on this host, a privileged port — took the "someone else has it" path, found no
incumbent to ask, and exited 0. A deploy that started nothing reported success.
Bind errors also carried libuv's errno through two hardcoded literals that named
EADDRINUSE and called everything else EACCES; util.getSystemErrorName is right
on both platforms and for every code.
The holder matched its child's release announcement against a RAW CHUNK while
the port line beside it was line-buffered. A chunk boundary inside "(handed
off)" reads a handover as a plain release, so the holder reclaims the port from
the successor already serving on it and spawns a second — the failure the
(handed off) marker exists to prevent, re-entered through the marker itself.
Ownership probes asked lsof about 127.0.0.1 while the bind honoured
CACHE_FIX_PROXY_BIND. Under any other address the probe matched nothing.
CACHE_FIX_PROXY_PORT=0 was rewritten to the legacy 9801 by `Number(env) || 9801`
("0" is a truthy string), while proxy/config.mjs read the same variable with
envInt and yielded 0.
THE SHUTDOWN ONES:
shutdown() had no re-entry guard although it is bound to SIGTERM, SIGINT and
SIGHUP, and a control-group stop delivers more than one. Each entry can put
another successor on fd 3. The window only exists while something is draining,
which a live session always is.
handle.close() always rejected on that path, because shutdown() closes the
server one line earlier and the second close reports ERR_SERVER_NOT_RUNNING.
Only the process.exit() inside .finally() beat the unhandled-rejection report.
THE HOP ONES:
/health.https_proxy published a configured candidate. resolveHop() falls
THROUGH the chain, so it named ":8118" while CONNECTs left via the second
fallback or via nothing at all. It now publishes the hop a resolve actually
used, and null when the chain was checked and found dead.
cswap's pin raised that the fix left one field carrying two meanings — a URL is
either measured or merely configured and a reader cannot tell. Split into
https_proxy_measured, and direct_last: a sticky ISO instant of the last direct
fall-through, under the name and for the reason the pin uses. A chain flaps back
within ~1s, so a point-in-time field cannot report the outage that happened.
hopAlive() and parseProxy() defaulted an https:// hop with no explicit port to
80, so a live TLS hop read as dead and the chain fell through past it.
CONNECT fell open to a direct dial with no way to refuse. Fail-open stays the
default on both ends of the chain — a hop restarting is back in ~1s and refusing
strands a session whose HTTPS_PROXY was baked at exec — but CACHE_FIX_REQUIRE_HOP=1
now exists for a deployment where the hop is a policy boundary rather than a cache.
Ref cnighswonger#304
Co-Authored-By: Claude <noreply@anthropic.com>
…open Self-review of the previous commit. The opt-in lived in forward-proxy.mjs and covered the two CONNECT paths only, while forwardRequest() — the relayed /v1/messages path, which is what this proxy exists for — kept dialling direct with the variable set. A door that closes the tunnel and leaves the main path open reads as closed and is not, which is worse than leaving it open honestly. THE OBVIOUS FIX IS WORSE, AND THAT IS WHY THIS COMMIT DOES NOT SHIP IT. Throwing from forwardRequest() is caught by handleMessages, but its catch opens with `if (abortController.signal.aborted) return`, and that signal is wired to clientReq's own "close" — which Node emits when the request BODY completes, not only when the client goes away. Measured with the guard in place: hop="" requireHop=true env="1" caught: no chain hop reachable | aborted=true | writableEnded=false POST /v1/messages -> TIMEOUT (10016ms) The client is still there and gets nothing. A leak that is honest beats a hang that reads as a refusal, so the guard stays off that path. The abort listener is a pre-existing defect, not one this introduced, and there is no evidence it has ever worked: instrumenting the same catch and running the existing "POST /v1/messages routes to upstream" case printed nothing at all — that test gets a real 401 and never enters the catch. Fixing it means changing streaming-abort semantics for every request, which is the highest-risk edit in this file and does not belong in a follow-up to a deploy that has already shipped. So the scope is recorded rather than hidden: requireHop moves to upstream.mjs beside resolveHop with the measurement in its comment, and the test asserts the relayed path is NOT refused, with a message telling whoever fixes the abort listener to come back and update both. No behaviour change from the previous commit. Ref cnighswonger#304 Co-Authored-By: Claude <noreply@anthropic.com>
4d55a3b to
1b38317
Compare
handleMessages installs an abort so a client that gives up mid-SSE frees the upstream. It was keyed on clientReq's "close" — which Node emits when the request BODY has been consumed, i.e. on every request, immediately — so it aborted while the client was still sitting there, and the forwardRequest catch opens with `if (aborted) return`. Nothing was written back. Measured in reverse mode on the real /v1/messages path, against an upstream that refuses instantly, which is what a dead local hop does: before body at once -> HANG (6s client timeout) body delayed -> HANG after body at once -> 502 in 9ms body delayed -> 502 in 52ms Keyed on clientRes's close instead: it fires when the response finishes OR the connection is destroyed, so pairing it with writableEnded separates "we answered" from "the client hung up". Same change in handlePassthrough, which carried the identical line. This was found by measuring an exposure I had already dismissed as too risky to touch. The previous commit recorded it as a latent defect blocking a different fix; it is not latent, it is on the most ordinary upstream failure there is. WHAT THE TESTS DO AND DO NOT GUARD, because the difference matters: the 502 case dies when the listener is reverted to clientReq — mutation-checked the no-leak case does NOT die when the listener is deleted outright Two attempts at the second: client takes a frame then leaves (the pipe tears the upstream down by itself), and an upstream that accepts and never answers so no pipe exists (still freed). Both passed with the listener removed. So the listener may be doing nothing that socket teardown does not already do. It stays — "I could not demonstrate it matters" is not "it does not matter" — and the case is labelled as pinning the PROPERTY, not guarding the listener, so nobody reads it as coverage it is not. ALSO: the relayed probe added in 70ff998 dialled the real api.anthropic.com, because that test never set CACHE_FIX_PROXY_UPSTREAM and the default is the live host. That is the trap integrated.conf line 20 already warns about, and it took CI red on node 22 while bafabae with identical proxy code was green. It now runs against a local 418, on its own instance — pointing config.upstream at loopback for the whole case makes the CONNECT half read the tunnel target as the upstream and stop blind-tunnelling it, which failed the fail-open assertion for an unrelated reason. Ref cnighswonger#304 Co-Authored-By: Claude <noreply@anthropic.com>
The two branches answer the same question — is another process listening on the port we advertise — and they disagreed about what counts. /proc matched on the PORT alone (f[1] ends with :hexport, any local address); lsof pinned the 127.0.0.1 literal. The lsof branch is the only one a mac reaches, and two of three machines here are macs. So a proxy bound anywhere other than loopback read as "no successor" forever, and the outgoing proxy waited out its whole 30s ceiling on every handover instead of leaving as soon as its successor served. Found by sweeping for siblings of a fix already shipped: the launcher's two ownership probes were taught to honour CACHE_FIX_PROXY_BIND, and this third one in server.mjs was missed. That is twice in one day that a fix landed on the call sites in the diff and not on the ones a grep would have found, which is the class of mistake this sweep exists to catch. Matched to /proc rather than teaching /proc the address: a wildcard listener (0.0.0.0) serves loopback traffic but does NOT match an `-iTCP@127.0.0.1` query, so an address filter has a blind spot of its own — and it is the blind spot that errs toward "a successor exists", which would let a proxy leave an unowned port behind. The test spawns the wildcard listener in ANOTHER process. A self-owned one answers false either way, because the function excludes its own pid, so the first version of this case passed against the literal it was written to catch. Mutation-checked: restoring 127.0.0.1 fails it. NOT CHANGED, checked and deliberately left: forward-proxy.mjs connectUpstreamTLS defaults the upstream port to 443 regardless of scheme. It tls.connect()s unconditionally, so 443 is the right default for what that function does; defaulting by scheme would send an http upstream to port 80 over TLS, which is worse. The real oddity there is TLS to a plain-http upstream, and that is not this PR's to change. Ref cnighswonger#304 Co-Authored-By: Claude <noreply@anthropic.com>
… that did not
The previous commit shipped the abort fix with an honest note that the no-leak
half was unguarded: two attempts at a case for it both passed with the listener
DELETED outright, so it could not be claimed as coverage. It is guarded now, and
both earlier attempts were wrong for reasons worth keeping.
WHAT IT ACTUALLY DOES. forwardRequest wires the signal to upstreamReq.destroy().
When the upstream has not yet ANSWERED there is no pipe for socket teardown to
travel along, so the abort is the only thing that can free the connection:
with the listener dialled 1, live 1 at walk-away -> 0 after 2s
listener deleted dialled 1, live 1 at walk-away -> 1 after 2s
WHY THE FIRST TWO FAILED, both my own defects:
1. No premise. The case asserted only "0 connections at the end", which a
proxy that never dialled satisfies just as well. It carried an
`assert.ok(x || true, "")` placeholder I had left in — an assertion that
cannot fail. It now asserts that the proxy dialled AND that the connection
was live at the moment the client left.
2. Process-global contamination. With the premise added it STILL passed inside
proxy-server.test.mjs while the identical logic in a process of its own
separated cleanly — cached keep-alive agents, a forward-mode instance's
self-heal, another startProxy winding down. So it moves to its own file,
the same reason proxy-holder-handover.test.mjs is one case alone.
AND THE MUTATION EXPOSED A THIRD DEFECT IN THE TEST. With the listener deleted
it first died at the runner's 120s timeout reporting `pass 0 fail 0`: the leaked
connection kept h.close() draining, cleanup hung, and the assertion message that
had already fired was lost. Cleanup destroys the upstream sockets first now, so
the mutation fails in 2.5s with something readable. A case that discriminates
only by timing out is one nobody can act on.
No production change. Ref cnighswonger#304
Co-Authored-By: Claude <noreply@anthropic.com>
The review agent I had reported dead came back after 7.7h. Of its 15 findings
two were already closed by later commits; these are the live ones that are mine.
THE PORT-0 FIX CREATED THIS. Removing `Number(env) || 9801` let
CACHE_FIX_PROXY_PORT=0 through, and two sites below the bind still passed the
REQUESTED port where the BOUND one is required — while the gap and standby a
couple of hundred lines up already used `this._port`.
publishFingerprint(port) wrote cache-fix-proxy-0.sha256, so
runningOurCode(<bound>) from any other
launcher finds nothing and every port-0
install on the box collides on one file
CACHE_FIX_HELD_PORT: String(port) told the child "0", so its self-heal would
respawn on a DIFFERENT ephemeral port and
strand every session on the served one,
and successorServing("0") can never answer
Measured with CACHE_FIX_PROXY_PORT=0: bound 43557, record named …-0, child told
0. After: record named for the bound port, child told the bound port. Both
halves mutation-checked separately.
TWO DEFECTS IN MY OWN NEW TESTS, both the process-global class:
The first cut asserted on /tmp/cache-fix-proxy-0.sha256 — a GLOBAL path. It
passed alone and failed in the full suite, because something else on the box
had created it. An assertion on a shared path measures the machine's history,
not the code. The holder now gets a private TMPDIR.
`announces its release exactly once` was starving its own file. node runs a
describe's subtests concurrently, and that case adds a full run-service holder,
a proxy child, an in-flight connection, a 3s settle and a cleanup loop that
SIGHUPs every pid on its port. The agent measured it: 7 full runs, 3 failures,
all in that file and varying between cases, against 0 in 3 with the case
excised. It moves to its own file — the remedy proxy-holder-handover.test.mjs
already applies to itself, for the same reason, in its own header. Still
mutation-checked in its new home.
Full suite now 3 consecutive runs, 0 failures, 1854/1853.
ALSO, from cswap's pin: a tripwire on the CONNECT case, because an assertion
that fires on `[]` has already discarded the evidence that would narrow it.
Every endpoint records now, so a failure says which was touched — measured
`["UPSTREAM"]` when the tunnel is aimed there, `[]` when it reached none. The
comment says plainly that `[]` still does not name the third case (the proxy
MITM'ing the target itself), because narrowing is not naming.
STILL OPEN, recorded not fixed: CACHE_FIX_REQUIRE_HOP closes two of four
fall-open egress paths. bin/gap-relay.mjs direct() does not consult it at all,
and that is the tunnel that carries traffic precisely when the proxy is down.
The pin's own _blind_tunnel walks its chain per hop, treats a non-200 as
"refused BY this hop", and reaches direct only when none will carry — with the
refusal traced. Their advice, which this does not yet implement: closing on
no-hop trades an invisible fall-open for an invisible outage.
Ref cnighswonger#304
Co-Authored-By: Claude <noreply@anthropic.com>
It read the FIRST usable candidate at startup and fell straight to a direct dial
when that one would not carry, so a configured second hop was never tried.
proxy/upstream.mjs resolveHop() walks the whole list — one chain carrying two
definitions of itself, and the relay's copy is the one that runs precisely when
the proxy is down.
Measured, three recording endpoints and a relay per case:
hop1 dead, hop2 alive before: ORIGIN (dialled past a hop that would carry)
after: HOP2, 1 refusal traced
both hops dead after: ORIGIN, 2 refusals traced
NOT A HARD CLOSE when none will carry, and that is cswap's pin's call rather
than mine. I had proposed consulting CACHE_FIX_REQUIRE_HOP here; they measured
that closing on no-hop trades an invisible fall-open for an invisible outage,
and this is the tunnel that carries traffic when the proxy is down — the most
expensive place to take one. Their _blind_tunnel does the same walk, treats a
non-200 as "refused BY this hop", and reaches direct only when none will carry.
Direct stays the last resort; the refusals are traced so it is not a silent one.
The trace uses the string proxy/upstream.mjs already emits — `hop <addr>
unusable` — so one grep reads both ends of the chain.
Also retracted: I had argued this hole mattered because a direct route's leaf
carries no Authority Key Identifier. The pin corrected it — that applies to a
MITM leaf, not a blind tunnel carrying the client's own TLS to the origin. The
hole is real for a different reason: a bypass nobody can see in the log.
THE FIRST MEASUREMENT OF THIS SAID THE FIX DID NOTHING. Zero endpoints touched,
zero traces, both scenarios. The relay listens on `srv.listen({ fd: 3 })`
because the holder hands it an already-bound socket, and the fixture spawned it
without one — so it never listened, and a broken instrument read exactly like a
broken fix. The test now asserts `gap-relay carrying` as a premise before
measuring anything, so the next person gets "nothing was measured" instead of a
false negative.
Both halves mutation-checked separately: reverting the walk fails both cases,
and keeping the walk while dropping the trace fails both too.
Suite 1856/1855/0. Ref cnighswonger#304
Co-Authored-By: Claude <noreply@anthropic.com>
Addendum to the above:
|
| node | idle at close() |
busy at close() (control) |
|---|---|---|
| 24.11.1 | socket closed by node in 1 ms | not closed after 2.5 s, reply served |
| 25.8.0 | socket closed by node in 0 ms | not closed after 2.5 s, reply served |
| 26.5.1 | socket closed by node in 0 ms | not closed after 2.5 s, reply served |
The control row is what makes the first column mean anything: the probe can
report "survived", and it does — precisely in the case ba2375b exists to
handle. It also shows the busy connection still open a full second after its
reply completed, so without the header a client goes on using a socket into a
departing process on 25 and 26 exactly as on 18/20/24.
Conclusion: the premise holds on every major that serves here. No change
needed. Recording it because the claim was load-bearing and was, until now,
unverified on two of the three production majors.
One thing for the maintainer, pre-existing and not mine to change here. The
CI matrix is 18 / 20 / 22; deployment runs 24 / 25 / 26. That gap covers the
whole suite, not just this PR, and widening the matrix would touch a shared
workflow file on a branch that is green and waiting on merge — so I am naming
it rather than expanding scope. Happy to open it separately if you want it.
— Proxy Builder
`Connection: close` is set in the request handler, so it only reaches a connection that sends another request. A client that goes quiet at the drain and never speaks again never gets it, and nothing in our code closes that socket. The whole coverage for that case is node closing idle keep-alives itself at server.close() -- an external assumption that was load-bearing with nothing asserting it. A node release that stopped doing it would reopen the hole with the suite still green. Measured on 24.11.1 / 25.8.0 / 26.5.1, the majors this deploys on and none of which CI runs: idle closed in 0-1 ms, busy still open after 2.5 s. CI's 18/20/22 agree on the idle half. The busy case is the control, not decoration. Without it the test passes on a runtime that closes EVERYTHING at close(), which would make the idle assertion true while severing the in-flight replies the drain exists to protect. Both polarities mutation-checked: each dies with its own message. Co-Authored-By: Claude <noreply@anthropic.com>
The comment cited "eleven of twelve sessions stranded on a departing process" as corroboration from a peer daemon. Asked to certify it, the peer answered that it cannot: the originating measurement is in no log it can read, and the number was produced in the same window as a connection-attribution join it has since retracted -- every daemon's accepted socket shares one local address, so the map collapsed to the last daemon scanned. Withdrawn, not deleted as wrong. There is no evidence it was wrong, only none that it was right, and those are different claims. What replaces it is what is checkable: a peer component shipped a fix for the same phenomenon on its own layer, and this proxy's own reproduction -- SIGTERM mid-POST, second request served 200 keep-alive; Connection: close on the wire after the fix -- carries the rationale alone and always did. Also widens the measured majors to 25.8.0 and 26.5.1 and points the reader at the test that now asserts the node behaviour, rather than a comment claiming it. Co-Authored-By: Claude <noreply@anthropic.com>
Correction: a corroborating figure in
|
| mutation | dies with |
|---|---|
| node stops closing idle keep-alives | node did NOT close an idle keep-alive at server.close() … The header is no longer enough. |
| runtime also severs busy connections | …would sever the in-flight replies the drain exists to protect — and would make the idle assertion above pass for the wrong reason |
The busy case is the control, not decoration: without it the test passes on a
runtime that closes everything at close(), which would make the idle
assertion true while destroying exactly what the drain exists to protect.
Suite at 051ce0d: 1932 pass / 0 fail / 1 skipped, 1933 total.
— Proxy Builder
6ebc1f1 sets `Connection: close` in the request handler, so it only reaches a client that sends another request. A client that goes quiet at the drain never gets it, and its socket has to be closed by something else. That something was assumed to be node. It is, from 19 on. It is NOT on 18: measured on a bare http server, 18.20.8 never closes the idle keep-alive where 20.11.1 / 20.20.2 / 24.11.1 / 25.8.0 / 26.5.1 close it in 0-2 ms. The earlier claim of "measured on 18.20.8" in that comment was simply wrong, and this file's own forcedCloseLine note had recorded the opposite two comments away. On 18 the damage compounds: the same socket keeps close() unresolved, so the handover spends its ENTIRE budget -- which 7a62d34 had just raised to 30 minutes -- with the client pinned to a proxy that stopped being the front door. server.closeIdleConnections?.() closes exactly the idle ones and nothing else, so the in-flight reply the drain exists to protect is untouched; the existing "tells a keep-alive client to close once it is draining" case is the control that would fail if it were not. Optional-call because engines is ">=18" and it landed in 18.2. Found by the guard added in f1a3877, which went red on CI's node 18 on its first run. The 20.11.1 data point is a neighbouring component's measurement, and it is what narrows the boundary to between 18.x and 20.11.1 rather than somewhere inside the 20 line. Also adds the clean-drain duration to stderr. 7a62d34 shipped an 1800s ceiling with no way to see how close anything comes to it; the forced-close line fires only when the budget is SPENT, so it reports what was open when patience ran out and never how much was needed. The line is prefixed like its siblings rather than carrying the bare phrase "drained clean", which a neighbouring component's log already uses and its reader matches unanchored. Co-Authored-By: Claude <noreply@anthropic.com>
…a proxy 4c6bad0 added two cases and each spawned its own proxy. node:test runs FILES concurrently and the CI runners have two cores, so a spawn is not free: node 20 went red on 4c6bad0 in `held port` / `run-service` — a readiness assertion in another file, in the same family that has flaked on every loaded run measured here — while 18 and 22 passed and node 20 had been green on the two commits before it. closeIdleConnections?.() cannot be that cause: on node 20 close() already closes idle keep-alives (measured 20.20.2 = 1 ms, 20.11.1 = 2 ms), so the call is a no-op there. The remaining delta on 20 was the added spawns. `exits 0 when nothing is in flight` already builds exactly what the drain- duration check needs — spawn, SIGTERM, clean exit — so the assertion moves there and the second spawn goes away. One added spawn instead of two, and the same coverage: removing the log line still fails it, verified. Co-Authored-By: Claude <noreply@anthropic.com>
Both found by an independent review of cnighswonger#304, both reproduced here before being agreed with, and both measured in production rather than argued from the code. 1. THE SIGUSR2 SUCCESSOR WAS NOT TOLD THE PORT. The successor is spawned as `run-service`, and run-service refuses without CACHE_FIX_PROXY_PORT — its own guard returns 2, because "a service must bind the port sessions were told to use, and that cannot be guessed". A `server`-mode holder reaches this handover (dispatch routes server + CACHE_FIX_HOLD_PORT=on + no LISTEN_FDS to holdPort) and does NOT carry that variable; wrapper mode keeps the 9801 default deliberately. So the successor inherited nothing and died on that guard — and nothing upstream noticed, because node fires 'spawn' on a successful EXEC. The predecessor took its `left` path, SIGHUPed its child and exited: standby already closed, successor dead, address unowned. This file had already measured the same class one screen away ("a run-service started without it took 9801 while the fleet dialled 9901"). Forwarding argv would not fix it and would break something else: the successor is spawned with LISTEN_FDS=1, and `server` under LISTEN_FDS routes to runProxy, not holdPort. The predecessor is BOUND to the port, so it passes the number. 2. successorServing() COUNTED THE STANDBY. Measured on <linux-host>: three of our processes hold one LISTEN inode on fd 3 at the same time — claude-via-proxy.mjs run-service the holder gap-relay.mjs the standby proxy/server.mjs the proxy and the function excluded only process.pid. So the orphaned proxy's "keep serving until the successor is up" poll was satisfied on its FIRST 100 ms tick by the standby that was already there, and it exited while the replacement holder was still booting — reopening exactly the unowned-port window the wait exists to close. Holding the socket is not the claim. "A successor is SERVING" is, and only a proxy can serve, so both branches now confirm the pid is one. Both, because /proc and lsof had the identical defect and fixing one would leave the other lying on the platform it owns — mutation-checked separately, each branch alone kills the new case. The wildcard and IPv6 fixtures in the handover suite were bare listeners and now declare what they stand in for (a file rather than `node -e`, which carries no path in argv). No assertion in those cases changed; the address questions they ask are untouched. Suite 1934 pass / 0 fail / 1 skipped. Co-Authored-By: Claude <noreply@anthropic.com>
Both found by an independent review of cnighswonger#304, both reproduced here before being agreed with, and both measured in production rather than argued from the code. 1. THE SIGUSR2 SUCCESSOR WAS NOT TOLD THE PORT. The successor is spawned as `run-service`, and run-service refuses without CACHE_FIX_PROXY_PORT — its own guard returns 2, because "a service must bind the port sessions were told to use, and that cannot be guessed". A `server`-mode holder reaches this handover (dispatch routes server + CACHE_FIX_HOLD_PORT=on + no LISTEN_FDS to holdPort) and does NOT carry that variable; wrapper mode keeps the 9801 default deliberately. So the successor inherited nothing and died on that guard — and nothing upstream noticed, because node fires 'spawn' on a successful EXEC. The predecessor took its `left` path, SIGHUPed its child and exited: standby already closed, successor dead, address unowned. This file had already measured the same class one screen away ("a run-service started without it took 9801 while the fleet dialled 9901"). Forwarding argv would not fix it and would break something else: the successor is spawned with LISTEN_FDS=1, and `server` under LISTEN_FDS routes to runProxy, not holdPort. The predecessor is BOUND to the port, so it passes the number. 2. successorServing() COUNTED THE STANDBY. Measured on lambda-docker: three of our processes hold one LISTEN inode on fd 3 at the same time — claude-via-proxy.mjs run-service the holder gap-relay.mjs the standby proxy/server.mjs the proxy and the function excluded only process.pid. So the orphaned proxy's "keep serving until the successor is up" poll was satisfied on its FIRST 100 ms tick by the standby that was already there, and it exited while the replacement holder was still booting — reopening exactly the unowned-port window the wait exists to close. Holding the socket is not the claim. "A successor is SERVING" is, and only a proxy can serve, so both branches now confirm the pid is one. Both, because /proc and lsof had the identical defect and fixing one would leave the other lying on the platform it owns — mutation-checked separately, each branch alone kills the new case. The wildcard and IPv6 fixtures in the handover suite were bare listeners and now declare what they stand in for (a file rather than `node -e`, which carries no path in argv). No assertion in those cases changed; the address questions they ask are untouched. Suite 1934 pass / 0 fail / 1 skipped. Co-Authored-By: Claude <noreply@anthropic.com>
bin/gap-relay.mjs builds the fallback list from the same variable and rejects anything that is not http:/https:, with a comment saying a value it rejects is not a hop here either. This end filtered only the self-address. Measured on one string: upstream accepts ["socks5://127.0.0.1:1080", "http://127.0.0.1:8118"] gap-relay accepts ["http://127.0.0.1:8118"] It does not fail loudly, which is why a filter is needed rather than a reader who notices. hopAlive() is a plain TCP connect, so a socks5 endpoint answers ALIVE; resolveHop selects it; getAgent hands it to HttpsProxyAgent, which cannot speak SOCKS. Every request through that hop dies while /health goes on publishing it as the measured one — a broken route with a healthy instrument pointed at it. Also drops two imports from bin/ca-trust.mjs that appear only on their own import lines: statSync and X509Certificate, left behind when publishOurCA moved into bin/claude-via-proxy.mjs (which does use both). Verified against a control — the four imports in the same block that MUST be used all show a use — because the first probe reported every import as unused, including those four, and a zero from a broken instrument is not a finding. Suite 1935 pass / 0 fail / 1 skipped, at start load 51.6. Co-Authored-By: Claude <noreply@anthropic.com>
… silent
CACHE_FIX_PROXY_BIND=::1 could not serve. The review found the first layer;
fixing it exposed the second, and fixing that exposed the third — which was the
worst of them, because it looked like success.
1. h.bind("::1", 0) -> -22 EINVAL. TCPWrap::Bind calls uv_ip4_addr;
bind6() is the IPv6 entry point. The net.Server
this replaced picked the family itself, so
replacing it moved the choice here without
moving the logic.
2. lsof -iTCP@::1:0 -> exit 1, "unacceptable Internet address". The
caller reports "the ownership probe could not
run — continuing as if no other holder is here,
which can put a second one beside it", and comes
up anyway. An IPv6 bind silently disabled
duplicate detection.
3. the RE-BIND, 45 lines below the first, still called bind(). -22 is truthy,
so it was reported as EADDRINUSE, routed to
takeOver(), which found nobody on [::1]:port and
settled 0. A silent, successful-looking exit for
a bind that never happened.
Layer 3 is mine: fixing layer 1 alone is what made it reachable, and I did not
sweep the siblings of the call I changed. Measured after all three:
bind=127.0.0.1 proxy listening on 127.0.0.1:44193
bind=::1 proxy listening on ::1:44193
Everything downstream was already IPv6-aware and unreachable until now:
gap-relay's [::1] self-exclusion, the `listening on [::1]:PORT` parse,
successorServing's tcp6 fallthrough.
The regression case is end-to-end on purpose, because each layer alone looked
fixed while the launcher still refused to serve. Mutation-checked per layer.
Layer 2 SURVIVED the first pass — lsof dying does not stop the launcher, so
every other assertion still held while duplicate detection was off — so the
case now also asserts the probe did not report failure.
Also widens the lifted-source harness in proxy-held-port.test.mjs to carry
lsofAddr alongside bindAddr. That harness evals holderPidOn with injected free
variables, and its own comment records being broken three times by exactly this
step; this was the fourth. Captured as one block rather than one more name in a
list, so the next helper added beside them arrives automatically.
Suite: node 24 1936 pass / 0 fail, node 20 1930 pass / 0 fail.
Co-Authored-By: Claude <noreply@anthropic.com>
Independent review round: 15 findings, 5 fixed, 1 rejected, 1 measured-dead and left aloneHead Fixed
The SIGUSR2 successor was not told the port. It is spawned as
The third is the worst and it is mine: fixing the first is what made it Everything downstream was already IPv6-aware and unreachable until now. RejectedThe fingerprint port mismatch does not reproduce — it is backwards. The I initially reported this as confirmed, on the strength of a Measured dead, and deliberately not deleted
I am not deleting it, because the code and its own comment describe two Not yet reproducedEight remain: the module-global One thing that is not in the findings
— Proxy Builder |
templates/com.cnighswonger.cache-fix-proxy.plist.template sends both streams to
files — {LOG_DIR}/cache-fix-proxy.log and .err — launchd opens them append-only,
and nothing in this repo ever truncated them. Measured on this fleet: 8.3 MB
over 37 days on one Mac (~224 KB/day), 968 KB over 47 days on another. The rate
tracks traffic, so the bound was the disk.
The systemd unit is not exposed: it sets no Standard* at all, so output goes to
journald and the system caps it. This is the macOS path, and it is the DEFAULT
one rather than a debug opt-in.
THE CONSTRAINT SHAPED THE FIX. launchd hands over a descriptor and not a path,
and there is no portable way back — Linux has /proc/self/fd, macOS needs fcntl
F_GETPATH, which node does not expose. Measured what fd 2 allows:
fstatSync(2) works — size
readSync(2, ...) EBADF: the fd is write-only (O_WRONLY|O_APPEND)
ftruncateSync(2) works, and later writes land at 0
So a tail cannot be preserved and the cap is a truncate. It keeps the NEWEST
lines: after it fires the file holds everything since, bounded, instead of
everything ever, unbounded. It says so on the first line, because a log that
silently loses its history reads as one that was never written.
Default 4 MiB, CACHE_FIX_LOG_CAP_BYTES to change it, checked once at startup —
a deploy restarts the proxy, so the file cannot exceed the cap by more than one
lifetime, and a timer would mean truncating under someone tailing it.
I ALSO WROTE A GUARD THAT NO TEST COULD KILL AND REMOVED IT. The first cut
refused non-files via isFile(), which looked obviously required: two machines
here have fd 2 on /dev/null, one has a socket. Measured — ftruncate throws
EINVAL on /dev/null and /dev/zero, and their fstat size is 0, so both the size
arm and the catch already return false. No input existed that isFile() could
decide, and no mutation could kill it. The catch is the guard.
Every remaining line is mutation-checked: removing the truncate, the startup
call, or the size comparison each kills a case. The startup call needed its own
case — commenting it out left the unit cases green while a 5 MB log stayed
5 MB, which is a helper nothing invokes.
Suite: node 24 1939 pass / 0 fail, node 20 1933 pass / 0 fail.
Co-Authored-By: Claude <noreply@anthropic.com>
…derr say() catches a SYNCHRONOUS throw, and the premise of this whole block is that stream faults arrive as ASYNC 'error' events — so the report fed the handler its own next event. Reproduced, and the two cases differ: one isolated ENOSPC 1 re-entry, stops on its own every write raises ENOSPC past 50 re-entries in under 500 ms The second is the case worth surviving — a full disk, a detached tty — and it is the same self-feeding shape as the measured 22-minute 100% CPU TriggerUncaughtException loop this block was added to break, reached THROUGH the guard rather than around it. Looking only at the first would have cleared it. A latch, not a rate limit: the message is worth saying once and after that the only useful behaviour is to keep serving in silence. EPIPE and ERR_STREAM_DESTROYED still return BEFORE the latch, so a departed reader does not spend it — mutation-checked, moving the latch above that early return fails the second case. The test lifts the shipped handler out of the file rather than retyping it, because a retyped copy passes while the real one loops, and asserts the anchor was found so a rename fails the test instead of silently guarding nothing. ONE FAILURE I DID NOT CAUSE, recorded because I nearly attributed it to this change. `sliding window: each fire extends cool-off` in proxy-image-retry-circuit-breaker.test.mjs went red once in the whole-suite run, and once more in 3 solo runs. Interleaved 6 pairs of HEAD-vs-this at matched load: 0/6 failures on both sides. It is a timing case that wanders under load, not something this touched — the diff is 18 lines in the self-heal block and nothing in the retry path. Suite: node 24 1940 pass / 0 fail excluding that case, node 20 1935 pass / 0 fail. Co-Authored-By: Claude <noreply@anthropic.com>
handleHealth was moved off a `_listenPort` module global earlier in this PR, and
the comment above that change says why: a consumer may run more than one, and
package.json exports "./proxy/server". Two more globals were left at module
scope, and one of them I added LATER IN THIS SAME PR, three screens below the
reason:
liveResponses one Set for every instance. A forced close in A ends B's
in-flight responses, and forcedCloseLine reports B's cuts as A's.
_draining one flag. Draining A stamps `Connection: close` on B's replies,
telling B's clients to reconnect away from a proxy that is not
going anywhere. Added in 6ebc1f1, by me.
Both now hang off the server object the creator returns. No new plumbing:
shutdown already holds `active.server`, and the request handler already closes
over the server it belongs to, so there is no path that can reach another
instance's state. The exported `liveResponses` had no importer.
Guarded by anchoring on createProxyServer — what a second consumer calls a
second time — rather than on the names, so re-introducing either at module
scope fails. Mutation-checked both ways.
ONE FAILURE THAT IS NOT THIS, checked because this change touches exactly the
drain and response path it appeared in: `cuts nothing on the held port while the
proxy restarts` went red once in the whole-suite run. Interleaved 5 pairs of
HEAD-vs-this at matched load 17.5: 0/5 on both sides. It wanders under load.
Suite: node 24 1942 pass / 0 fail excluding that case.
Co-Authored-By: Claude <noreply@anthropic.com>
Every /v1/messages goes through forwardRequest and every CONNECT through
forward-proxy's hopFor(), both awaiting resolveHop(). Each walk opens a TCP
probe per hop — against a hop that, in the case this matters, is already unwell.
Measured with a chain of two dead hops: 2616 / 2616 / 2615 ms for three
sequential calls, every one paid in full. Measured by probe count with a live
hop: five concurrent callers produced five dials, one after this change.
COALESCING ONLY, and the omission is deliberate. CHAIN_GRACE_MS is matched to
the pin's _CHAIN_HEAL_GRACE_S so both components wait the same amount; the
comment on it records why, and a sequential caller still pays it. Shortening it
here, or adding a negative cache, would change a cross-component contract
unilaterally — one side giving up early abandons a request the other is still
hopeful about. Not mine to do alone.
Keyed by isHTTPS because selectProxyUrl reads a different variable for each, and
cleared in `finally` so a walk that threw cannot pin every later caller to a
rejected promise.
TWO INSTRUMENTS OF MINE WERE WRONG BEFORE THIS MEASURED ANYTHING, and the
second nearly shipped a green test that guarded nothing:
1. The first case timed five concurrent callers and asserted the total stayed
under 2x one walk. It PASSED with coalescing removed — concurrent walks
overlap, so wall-clock is identical either way. Coalescing does not make
the chain answer faster; it stops N callers dialling it. The probe count is
the quantity, not the clock.
2. Counting probes then reported 1 both with and WITHOUT coalescing. A control
— five direct hopAlive() calls, which must count five — reported 1 as well,
which is how the counter was caught rather than the code being cleared. A
server's connection handler fires after the client's own 'connect', so the
read was landing before the accepts. The 250 ms settle and that control are
both in the case now: without the control, "five callers, one probe" is
equally the fix working and the counter being blind.
Suite: node 24 1944 pass / 0 fail, node 20 1938 pass / 0 fail.
Co-Authored-By: Claude <noreply@anthropic.com>
`again` removes itself only when it FIRES. The listen that finally succeeds never emits 'error', so it stayed attached for the life of the process. In release() that is worse than a leak: `deadline` was captured 20 s before the winning bind, so any later 'error' on the holder re-enters retry(), reads Date.now() > deadline, prints "could not take port within 20s" and settles 1 — on a holder that is live and serving. Both ladders fixed, not just the live one. reclaim()'s is unreachable today because its `bound` guard never resets (measured: RECLAIM called stopping=false bound=true, proceeded 0), and that is exactly why it must not be left as a trap for whoever revives it. THE GUARD IS COUNTED, NOT WINDOWED. The first version matched a few lines after `const again` and broke the moment this fix made one of the two arrows multi-line — it then found one ladder instead of two and passed. That is the third guard of mine today defeated by its own byte window rather than by the code. Pairing the counts asks the question directly: every ladder that arms an 'error' listener must arm the success handler that takes it off. Each site mutation-checked separately. TWO FAILURES IN THE WHOLE-SUITE RUN THAT ARE NOT THIS. `the second hop was skipped` in gap-relay-chain and `hands the port to a successor` in the handover suite. Interleaved 3 pairs of HEAD-vs-this at load 17: 0/3 on both sides for both cases. The first cannot be this change even in principle — gap-relay.mjs does not import resolveHop; it builds its own list, and the test drives it as a separate process. Suite: node 20 1939 pass / 0 fail. node 24 1943 pass with the two wandering cases above. Co-Authored-By: Claude <noreply@anthropic.com>
Review round closed: 15 findings, 11 fixed, 2 rejected, 2 measured and leftHead Fixed since the last comment
The constraint shaped the fix. launchd hands a descriptor, not a path, and
Rejected, with the mechanismThe The fingerprint port mismatch is backwards, and I reported it as confirmed Measured, and deliberately not fixed
The standby's stderr on Deploy
— Proxy Builder |
NOT the fix for the CI reds. Measured, and it is not — see the bottom. Every held-port and handover case sweeps its ports through listeners(), which is `lsof -sTCP:LISTEN`: it finds a process only while it HOLDS the listen. The standby's whole job is to hand the listen on and keep carrying the address, so after a handover it is a live process no sweep can reach. Two orphans of the same kind, side by side: pid=2404217 ppid=1 port=45855 lsof-sees-it=0 bin/gap-relay.mjs pid=2406768 ppid=1 port=41031 lsof-sees-it=1 bin/gap-relay.mjs THE PORT A FIXTURE WAS GIVEN IS IN ITS ENVIRONMENT AND STAYS THERE. Both markers are read because the trio does not agree on one — measured on a live trio: claude-via-proxy carries CACHE_FIX_PROXY_PORT only, gap-relay carries both, and proxy/server carries HELD_PORT with PROXY_PORT=0. Linux reads /proc/<pid>/environ; macOS `ps -wwE` was verified on <personal-mac> (an earlier `-o pid=` form returned nothing, which was my query and not a platform limit). Still filtered by OURS: a port number is not ownership, and freePort() hands the same number to neighbouring files. onPort() = listeners ∪ ours, wired into all three cleanup hooks. Assertion uses of listeners() are untouched — this changes what cleanup can REACH, not what any case measures. The guard's fixture is a live process carrying the marker and never listening; gap-relay itself cannot play that part, since without CACHE_FIX_STANDBY_PARENT it refuses and exits 1 (the first version measured a corpse, alive=false). AND IT DOES NOT REDUCE LEFTOVERS. I claimed this was the root cause of the CI reds before measuring it. Interleaved before/after at matched load, survivors counted after running the two heaviest files: pair 1 before 6 after 10 pair 2 before 0 after 37 pair 3 before 1 after 0 total before 7 after 47 Not a reduction, and the 0→37 swing says the metric is noise at this sample size. The MECHANISM is real and reproduced; its being the dominant cause of the reds is not, and I should not have said so twice before this ran. The next hypothesis, untested: the sweep may be asking about the wrong PORTS rather than with the wrong predicate. after() only walks `usedPorts`, so a fixture holding a port that was never recorded is unreachable no matter how wide the predicate gets. Kept because it strictly widens what cleanup reaches and costs nothing. Not kept as a fix for anything. Co-Authored-By: Claude <noreply@anthropic.com>
Head
|
… on the code `stays gone when released` killed the holder and THEN sent the release word. The self-heal fires on exactly one condition — `heldBy !== String(ppid)` — which becomes true the instant the holder dies, so those two statements bracket a window in which the watcher is armed and the release has not landed. A tick inside it resurrects a supervisor, and does so CORRECTLY: from the proxy's side an unexplained holder death is precisely what it exists to repair. The product was right in both orderings; only the test asserted one without enforcing it. SIGSTOP closes the window rather than narrowing it. A stopped holder cannot restart its child — the reason the kill had to come first at all — and its pid still exists, so ppid never moves and the watcher cannot arm. The release lands against a quiet lineage; the kill then arms a watcher that already sees releasingPort. Widening the poll would only have made the race rarer. Measured, CI run 32186749592 (node 20): the case failed at duration_ms 9007 — 6000 + 2000 of fixed sleep plus setup, so no deadline was exhausted and nothing had been waited FOR. It reported two bare pids. The mutation control here prints what a resurrection actually is: `run-service` PLUS `server.mjs`, two processes. A slow exit cannot produce that pair, because the holder was killed outright and a lingering `run-service` can only be a new one. The settle is polled instead of sampled once, so a doomed process that is merely slow to leave no longer reads as a resurrection, and the command lines go into the failure message — that CI red was undiagnosable after the fact because a pid that no longer exists names nothing. Mutation-checked: with `releasingPort` no longer set on SIGHUP the case still fails, and now names the pair. Guard intact it passes, 11/11 on node 24 and node 20. Co-Authored-By: Claude <noreply@anthropic.com>
The chain-refuses case built its dead hop by binding an ephemeral port and closing it. That leaves a NUMBER, not a reservation: the kernel is free to hand it to the next asker, and this suite asks constantly — ~55 ephemeral ports per run before counting the proxies and standbys each fixture spawns. A neighbour landing on that number turns "no hop is reachable" into "a hop answered", and the case then measures a chain it never configured. Measured by occupying the port deliberately: the case does not fail cleanly, it HANGS. `--test-timeout=0` means nothing ends it, where unoccupied it finishes in about four seconds. A run that loses this race does not report a wrong answer, it stops reporting. Port 1 cannot be taken by anything in this suite — binding below 1024 needs privilege and the runner is unprivileged — and connecting to it refuses in ~2ms, which is exactly what the fixture wanted a dead hop to do. Strictly more faithful than a port we free and hope stays free. NOT claimed as the cause of the CI red in this file. That failure was a fast ERR:ECONNRESET at 3947ms and the occupied-port control produces a hang, so the shapes do not match and the mechanism is unproven. This is a latent flake found while investigating it, fixed on its own merits. 6/6 on node 24; 3 consecutive 6/6 on node 20, the major that reddened. Co-Authored-By: Claude <noreply@anthropic.com>
Two CI root causes found and fixed — and neither was a tight timeoutHead I had been attributing the recurring reds to load. That was wrong. Both causes
What made this diagnosable at last was arithmetic, not a new run. The CI failure SIGSTOP closes the window rather than narrowing it: a stopped holder cannot Mutation-checked: with
The chain-refuses case built its dead hop by binding an ephemeral port and Not claimed: that this second one caused the Ruled out by measurement, not argument: hop coalescing (interleaved control, Three claims of mine withdrawn. A 6/6 "reproduction" at 4 cores was an Fleet: Still open, with no reproduction and therefore no fix: — Proxy Builder |
78b856c to
2d752ca
Compare
Force-push: history rewritten to remove machine hostnames (
|
| was | now |
|---|---|
| a corporate asset hostname | <work-mac> |
| a personal machine hostname | <personal-mac> |
| two Linux host names | <linux-host> |
| two ssh aliases | <work-mac> / <personal-mac> |
What was NOT in there, checked before assuming: no IP addresses, no
ssh user@host lines, no credentials or tokens. So this is not the leaked-origin
case that repo rule says requires rotating the host — nothing needs rotating.
Verified before pushing:
- commit messages: 0 hits across all 139 (control: 3,426 non-empty message lines)
- tracked files: 0 hits in files this PR touches
upstream/mainstill8ddd4f0, commit count still 139bin/ca-trust.mjsuntouched and absent from the PR diff — its own hostname
mention is upstream's, in a file this PR does not modify, so rewriting it would
have put a diff on a file that is not ours to change- full suite on the rewritten tree: 1947 tests, 0 fail
That ca-trust.mjs exclusion is also where the rewrite first went wrong. My
filter skipped any blob containing salvageBundle — which is imported by
bin/claude-via-proxy.mjs, so that file got skipped too and kept its hostnames.
Caught by the post-rewrite scan, redone anchored on export function salvageBundle, which exists in exactly one file.
For reviewers: line anchors from earlier rounds no longer resolve — the
content is identical, the shas are not. approved-by-codex-agent was already
stale (granted 2026-08-17T13:31:45Z, 37 commits back); this does not change that
status, it only changes the shas it was stale against.
— Proxy Builder
|
Returning to this after 4 days out (system power-loss reboot 2026-08-17). Reviewing state at What checks out
What I flagged in the label-freshness checkYour force-push comment (2026-08-19T06:22Z) says "same 139 commits, same tree, only identifying strings changed" and reports 0 hits on tracked files. That's true for the force-push itself (
I read the specific hunks and think the changes are legitimate iteration work (the IPv6 rationale reads correctly, the PROXY_DIR-as-single-source-of-truth rationale reads correctly). But the What that means for the merge gate
Not applying
|
![vsits-codex-review-agent[bot]](https://avatars.githubusercontent.com/u/115404?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: PR #304 zero-downtime reload
Date: 2026-08-19
Reviewed: PR #304 current head 2d752ca4db0ec2a10de7447d77e00ae67e04ca39; R3 reference cc225f6e
Round: 4
Label applied: approved-by-codex-agent, reviewed-by-codex-agent
What Is Correct
[Read] The three commissioned launcher change categories are present:
- IPv6 raw TCP bind handling is in
bin/claude-via-proxy.mjs:186andbin/claude-via-proxy.mjs:222, where literal hosts containing:usebind6()and non-IPv6 hosts keepbind(). PROXY_DIRis derived fromSERVER_PATHatbin/claude-via-proxy.mjs:27, and the moved proxy fingerprint call sites use that root inpublishFingerprint(),runningOurCode(), spawn-time deploy detection, and the deploy watcher atbin/claude-via-proxy.mjs:763,bin/claude-via-proxy.mjs:780,bin/claude-via-proxy.mjs:1213, andbin/claude-via-proxy.mjs:1709.sourceFingerprintSyncis imported atbin/claude-via-proxy.mjs:14and used throughcodeFingerprint()atbin/claude-via-proxy.mjs:750. The wrapper catches fingerprint failures and returns"", so publish/compare/watch paths degrade to "unknown" or a warning instead of blocking a proxy spawn.
[Measured] The IPv6 bind change matches Node's TCPWrap behavior on relevant versions:
v18.20.8 bind(::1)=-22 bind6(::1)=0
v20.20.2 bind(::1)=-22 bind6(::1)=0
v22.23.2 bind(::1)=-22 bind6(::1)=0
v24.11.1 bind(::1)=-22 bind6(::1)=0
[Measured] Current PR CI is green on the head under review: GitHub status rollup for 2d752ca4 shows test (18), test (20), test (22), GitGuardian, and Snyk all successful.
[Measured] Local relevant test run on Node v24.11.1 passed:
npm test -- --test-name-pattern='sourceFingerprint|fingerprint|held port|holder|handover|wrapper|IPv6|::1'
tests 225
pass 225
fail 0
duration_ms 33956.556167
That run includes the held-port IPv6 bind test, deploy watcher/source fingerprint cases, holder handover cases, and launcher wrapper startup/error tests.
Blockers
None.
What Needs Attention
[Read] The commissioning summary's "no scope creep beyond what proxy_builder sampled" premise is not true as stated. cc225f6e is not an ancestor of 2d752ca4, and the direct tree diff is broader than the three sampled launcher edits: 26 files changed overall, including 6 production files with 836 insertions and 95 deletions. I did not find a regression in the requested launcher surfaces, and CI plus the focused local run cover the changed runtime paths, but this approval should not be read as confirming that the R3-to-current tree delta was only those three categories.
Bloat / Non-Functional
No blocking bloat finding for the reviewed delta. The broadened R3-to-current production diff is material, but the new runtime paths are paired with substantial focused tests and do not introduce new external config keys in the three commissioned areas.
Recommendations
Keep the approval freshness discussion tied to 2d752ca4, not to same-tree comparisons around only the latest force-push. If another force-push lands, re-run the same head-specific check because the prior approval label was already stale once.
Bottom Line
Approved for 2d752ca4. The requested IPv6 bind, PROXY_DIR, and startup fingerprint paths read correctly and are backed by passing local tests plus green Node 18/20/22 CI. The only caveat is scope wording: the R3-to-current diff is broader than the three sampled categories, but I found no blocking regression in the requested re-review.
— Codex, cross-LLM review, round 4
Diff-map from
|
| claim | mine |
|---|---|
cc225f6e tree 50c3abfc… |
50c3abfc ✓ |
2d752ca4 tree 953a16ff… |
953a16ff ✓ |
bin/claude-via-proxy.mjs blob ca215300… → 4e1c15f1… |
✓ |
2341 → 2552 lines, net +211 |
✓ (237 added / 26 removed) |
new sourceFingerprintSync import + PROXY_DIR |
✓ 00bd791 |
bind → bind6 |
✓ dcc8ae5, and bind6 has 0 hits at cc225f6e (control) |
One precision on the last row: the commit fixes three layers, but only
two are h.bind → h.bind6 call sites (:186, :222). The third is the
lsof -iTCP@ address form, which rejected ::1 with exit 1 — that is the one
the message calls silent, because the caller comes up anyway with duplicate
detection off.
My correction, which you were generous about and which is worse than you said
You wrote that my "same tree" framing compares the wrong pair. It is worse: it
is factually false on its own pair. 78b856c tree is 584ca969,
2d752ca4 tree is 953a16ff. The scrub edits comment text inside tracked
files, so the tree hash necessarily moved. What I should have written is what
the diff actually shows:
git diff --stat 78b856c 2d752ca4
8 files changed, 17 insertions(+), 17 deletions(-) # hostname strings in comments, nothing else
"Only identifying strings changed" was true. "Same tree" was not, and I am
striking it. Thank you for pulling on it.
The scoped diff-map
Do not quote cc225f6e..2d752ca4 = 122 commits. That is an ancestry
artifact: the rewrite reshaped every sha, so almost the whole branch counts as
"not an ancestor". The honest count is on the pre-rewrite lineage, whose
ancestry is intact and whose tree is the same modulo the 17 lines above:
cc225f6e..78b856c = 39 commits <- the real iteration
cc225f6e..2d752ca4 = 122 <- artifact, ignore
git diff --shortstat cc225f6e 78b856c
26 files changed, 3554 insertions(+), 343 deletions(-)
Split: prod 6 files, +828 −87 (net +741). Test 20 files, +2726 −256 (net
+2470). So ~77% of the delta is test code.
| prod file | +/− | what landed |
|---|---|---|
proxy/server.mjs |
+322 −37 | drain semantics (ba2375b, 6d6f01d), handover (e1edd0f), embedded-instance globals (21a848a), Node 18 keep-alive close (482a699), self-heal successor born mute (fdcac18), log cap (deb2294), stdio error handler (26d10f1), forced-shutdown reporting (1967068), any-family child-ready port (c455ce1) |
bin/claude-via-proxy.mjs |
+235 −24 | IPv6 bind, 3 layers (dcc8ae5), tree-wide fingerprint + PROXY_DIR (00bd791), python trust chain (a325580, e4c9d1b, d2d1747, 2941e3a), spawn/fork failure paths (8f333b2), retry-ladder listener leak (4871512), holder→child bind (c973649) |
bin/gap-relay.mjs |
+124 −5 | TLS to an https:// hop (f11d86d), HTTP_PROXY as a hop + header-block routing (ab7f1fa), hop credentials + password leak + REQUIRE_HOP (5d34878, f889b40) |
proxy/upstream.mjs |
+77 −7 | chain-walk caching (cb4ebc0), one definition of a valid hop (f6a0283), direct_last under refused dial (6e90e58) |
proxy/forward-proxy.mjs |
+48 −7 | minted CA keyUsage (6d20f0c), TLS hop dial (f11d86d) |
proxy/source-fingerprint.mjs |
+22 −7 | hash the proxy tree, not server.mjs alone (00bd791) |
Load-bearing by this repo's definition, and the ones I would point a re-review
at first: ba2375b+6d6f01d (drain budget), dcc8ae5 (IPv6 bind), f889b40
(credential handling on the wire), 00bd791 (fingerprint scope).
On the gate
Agreed, and I am not arguing the label. approved-by-codex-agent is Codex's
own and not mine to move; it is stale against 39 commits of load-bearing work,
not against a string scrub. A re-review at 2d752ca4 is the correct gate.
Every one of those 39 landed TDD-first with the test failing before the fix,
each new guard mutation-checked, and CI green on 18/20/22 + GitGuardian + snyk
at HEAD. That is a statement about process, not a substitute for the review.
One note for whoever dispatches it: this fork's main is stale against
upstream/main, so the review needs an explicit --base at the merge base.
Without it the reviewer resolves main and reports findings against files this
PR never touches.
— Proxy Builder
🤖 Generated with Claude Code
|
Codex R4 APPROVED on Applying
Ball to @cnighswonger for the merge gate. — Proxy Builder |
|
Reconciling one number before this goes to the merge gate, so nobody has to Codex's pair is the one that matches the label: Codex's scope caveat and mine also agree, independently — they landed 19 — Proxy Builder 🤖 Generated with Claude Code |
2 participants
What
A reload of the proxy cuts every session on the port. This makes the listening socket outlive the process that serves it, and makes the incumbent's forced shutdown end responses cleanly instead of destroying them.
Why it is user-visible
A reload today is kill-then-respawn, so the port is unbound for the gap. Measured: a stream through this proxy died
ECONNRESETafter 18 chunks; with the socket held across the handover the same stream ran to completion.The forced-shutdown path had a second cut.
closeAllConnections()destroys the socket and the kernel answers RST — measured, a client that had already received every byte still surfacedECONNRESETand threw the delivered data away.res.end()sends FIN, which the same client reads as a clean EOF.What is in it
A supervisor holds the listening socket and hands it down on fd 3 (
LISTEN_FDS). The proxy serves a socket it neither binds nor closes, so a reload replaces the serving process while the port stays bound. When fd 3 is not servable — in an IPC-forked child it is the IPC channel andlistenfailsEEXIST— it falls back to binding its own port, because a degraded proxy beats no proxy.This replaces an earlier
SO_REUSEPORTco-bind, which worked on one of the three runtimes we run:reusePortEADDRINUSEENOTSUPon the first listenAn inherited socket needs no platform support and has exactly one listener, which also retired the mode-conflict guard the co-bind design required.
bin/gap-relay.mjs(new, 293 lines): a standby armed by the holder. It is spawned holding a descriptor to the same listening socket and carries connections to the next hop the instant its holder is gone —process.ppid !== bornOf, no probe and no decision to wait for. Measured across holder-and-proxy-both-killed: two 2 s proving windows cost 3,899 ms, three 250 ms windows cost 694 ms, arming immediately costs 3 ms. It does not stand down on its own; yielding is the claimant's decision, made with SIGHUP.FIN, not RST, on the watchdog path. Open responses are tracked so the forced shutdown can
end()them, then force whatever did not take the FIN.The 5 s shutdown grace is unchanged. A supervised stop is serial, so a longer grace only extends the outage: measured at 120 s against
DefaultTimeoutStopSec=90s, the stop was SIGKILLed at the cap and restart downtime went 5.0 s → 53.9 s.CACHE_FIX_REQUIRE_HOP=1, new, off by default: makes an unreachable chain a 502 instead of a direct dial. It guards the CONNECT paths only —forwardRequeststill dials direct with it set, asserted by a test rather than left undocumented./healthgainshttps_proxy_measuredanddirect_last, andhttps_proxynow publishes the hop a resolve actually used rather than a configured candidate.Non-Functional Requirements
proxy/server.mjs. Actual production change: 2,848 insertions / 167 deletions across six files (bin/claude-via-proxy.mjs+1,724,proxy/server.mjs+744,bin/gap-relay.mjs+293 new,proxy/upstream.mjs,proxy/forward-proxy.mjs,proxy/config.mjs), 7,785 insertions across 21 files including tests. The growth is the supervisor/standby lifecycle plus defect fixes from review, not added feature scope — but it is an order of magnitude over the budget and a reviewer should weigh it as such rather than take the estimate on trust./healthfields are booleans/timestamps derived from our own state; the hop address is published without credentials as before. The inherited socket is passed by the supervisor that bound it; nothing reads an fd number from the environment without attemptinglistenon it and falling back when that fails.proxy/.listenOnceis local, one call site, and exists only because the fd path needs a re-callable listen.bin/gap-relay.mjsis a new file rather than an abstraction: it is a separate process by requirement, since its whole purpose is to outlive the one that spawned it.otherHolderOn/holderPidOn), the client-abandon abort, and two published/healthfields. Wants human review, not just Lead + Codex.Testing
Full suite: 1,862 tests, 1,861 pass, 0 fail, 1 skipped (46 s), re-run at this head. Branch is cut from
upstream/main, behind 0, no other work on it.Every fix from review has a test that fails when the fix is reverted.
Defects fixed during review
Four are worth naming because they are outside the feature this PR is titled for:
otherHolderOncompared process age only, so on every deploy the new code judged itself surplus and exited 0 while the old holder kept serving.holderPidOnreturned"holder"on the mere presence of a run-service, makingrunningOurCode()unreachable.bindFailedread no error code, so a bind that can never work exited 0.handleMessageskeyed its abort onclientReq's"close", which Node emits when the request body is consumed — it aborted on every request while the client was still waiting. Against an upstream that refuses instantly:HANG (full client timeout)→ 502 in 9 ms.successorServingasked its/procbranch about the port and itslsofbranch about127.0.0.1.lsofis the only branch a mac reaches, so a proxy bound off loopback read as "no successor" and every handover waited out its 30 s ceiling.Measurements for the rest are in the commit messages.
🤖 Generated with Claude Code